Repository navigation
fix(marketplace): poll listings recorded as pending and document where the poll runs - #1508
Conversation
…e the poll runs A listing AWS gave no status for when it was created (or whose rollback cancel failed) is recorded as pending with its listing id and no attempt token. ListMarketplaceListingsToPoll only selected active and unsettled terminal rows, so such a listing never became active and a sale on it was never recorded. - ListMarketplaceListingsToPoll now also returns pending rows that hold a listing id and no client token. A pending row with a token is a list attempt in flight whose listing_id is the RI's previous listing, and is still left out. - RecordMarketplaceListingActive moves such a row to active (stamping listed_at when AWS reports a CreateDate) and the poller calls it for a pending row even without a CreateDate. - SettleMarketplaceListing also settles such a row. - Every write keeps its existing guards and re-checks the token, so a cancel or relist claim that commits while the poll waits on the row wins. The poll runs inside finalize_revocations, so docs/DEPLOYMENT.md now lists where that task is scheduled (Lambda, CloudFormation, AKS) and where it is not (Fargate, Cloud Run, GKE, Container Apps), and the schedule toggles say that turning them off also stops the poll. Superseded listings (gap 2) are not addressed: recording an earlier listing's settlement after a relist needs per-listing storage. Refs LeanerCloud#1494 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Warning Review limit reached
This review includes 11 billable files and costs up to $2.75. Or wait 59 minutes for your next included review. View limit details
Comment |
|
Independent review (IAmJSD's agent, fresh context, given only the diff, the issue and the PR text): VERDICT: APPROVE Blocking:
Non-blocking:
Attacked:
Body-stale: none. The PR correctly says Refs (not Closes), because gap 2 and the "before exposing proceeds" items are left open. Reviewer: rF1. Bar while the maintainers are away (set by IAmJSD as acting owner): one approving independent review on the head, two for money, auth and security paths. |
|
Local verification at |
|
Merged by IAmJSD's agent at IAmJSD's request (IAmJSD is acting owner while the maintainers are away), under the bar IAmJSD set: 1 approving independent review(s) on |
…ettlement table too The per-listing settlement write ignored LeanerCloud#1508's rule that a row mid-claim (pending with a list attempt's token) is left alone: a poll of the listing a claim had just kept was recorded in marketplace_listing_settlements while the claim was in flight, so SettleMarketplaceListing reported applied. - SettleMarketplaceListing share-locks the purchase_history rows that hold the listing after its own UPDATE and writes no settlement row while one of them is a claim in flight. The lock makes a claim that starts meanwhile wait, so nothing is written mid-claim. The listing is settled once the claim resolves (it is then polled from the kept row) or is released. - ListMarketplaceListingsToPoll leaves such a listing out of the settlement table branch as well. - The trigger also keeps the listing when a resumed claim (a released attempt whose token was kept, back in the listing's own state) makes the row pending again, and never copies a row that is already mid-claim. Refs LeanerCloud#1494 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…s own row (#1581) * fix(marketplace): keep each RI Marketplace listing's settlement in its own row purchase_history describes only an RI's latest marketplace listing, and a relist (or the claim that starts one) moves the row to the new listing on purpose (#1289). A listing that sold partially and was canceled before a poll saw the sale was then never polled again, and its proceeds were lost. - Migration 000153 adds marketplace_listing_settlements, one row per AWS listing id with the purchase_history row it was recorded on, the RI id, status, listed_at, proceeds, fee, currency and timestamps. It backfills one row per listing purchase_history records a settlement for. - Trigger purchase_history_keep_superseded_listing copies a listing into the table when the row stops describing it: its listing_id changes, or a list claim stores a token over it. ON CONFLICT DO NOTHING keeps a row the poller already wrote. - ListMarketplaceListingsToPoll also returns those kept listings, under the same rules as purchase_history rows, once per listing. - SettleMarketplaceListing writes, in one transaction, the purchase_history row while it still holds the listing (as before, so its columns stay the latest listing's settlement) and then the listing's own row, created or filled only while its amounts are unset. purchase_history is locked first, the same order a relist takes, so the two cannot deadlock. - GetMarketplaceListingProceedsTotals sums an RI's proceeds and fees over its listings from the table alone, so a sale is never counted twice. Closes #1494 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * fix(marketplace): leave a listing held by a list claim alone in the settlement table too The per-listing settlement write ignored #1508's rule that a row mid-claim (pending with a list attempt's token) is left alone: a poll of the listing a claim had just kept was recorded in marketplace_listing_settlements while the claim was in flight, so SettleMarketplaceListing reported applied. - SettleMarketplaceListing share-locks the purchase_history rows that hold the listing after its own UPDATE and writes no settlement row while one of them is a claim in flight. The lock makes a claim that starts meanwhile wait, so nothing is written mid-claim. The listing is settled once the claim resolves (it is then polled from the kept row) or is released. - ListMarketplaceListingsToPoll leaves such a listing out of the settlement table branch as well. - The trigger also keeps the listing when a resumed claim (a released attempt whose token was kept, back in the listing's own state) makes the row pending again, and never copies a row that is already mid-claim. Refs #1494 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * refactor(marketplace): read the mid-claim check through a helper Keeps govet's shadow check and gocritic's sloppyReassign both satisfied. Refs #1494 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>
Summary
This PR handles gap 1 of the issue and settles gap 3 by documenting it. Gap 2 (superseded listings) and the "Before exposing proceeds" checks are left open; see "Out of scope" below.
Refs #1494
Root cause
The RI Marketplace poller (#1289) reads the rows returned by
ListMarketplaceListingsToPoll. That query selected only two kinds of row:activelistings, andclosed/cancelledlistings withlisted_atset and no proceeds recorded.The list handler records a listing as
pendingin two cases: AWS gave no status when the listing was created (createdListingState), or the listing was created but neither persisted nor rolled back (keepUncanceledListing). Such a row holds the listing id and no attempt token. It was never selected, so it never becameactive, and a sale on it was never recorded. The store writes also refused it:RecordMarketplaceListingActiverequiredactive, andSettleMarketplaceListingrequiredactiveor the same terminal state. The settlement integration test even listed this row shape as excluded, labeled "mid-claim".Fix
Gap 1: pending rows that hold a listing id (
internal/config/store_postgres_marketplace_settlement.go,internal/api/handler_marketplace_poller.go)The store and the poller now handle a row that is
pending, holds alisting_idand haslisting_client_token IS NULL:ListMarketplaceListingsToPollselects it.RecordMarketplaceListingActivemoves it toactiveand stampslisted_atwhen AWS reports aCreateDate.listedAtnow keeps the storedlisted_at(COALESCE) instead of being written.activerow the statement behaves exactly as before and still never writeslisting_state.SettleMarketplaceListingsettles it toclosed/cancelledlike an active row.RecordMarketplaceListingActivefor a pending row even when AWS reports noCreateDate. Before, the poller skipped the write wheneverCreateDatewas missing. An active row without aCreateDatestill gets no write.A
pendingrow with a token is still excluded from all three statements. The marketplace-list handler is mid-claim on that row, and the row'slisting_idis the RI's previous, ended listing. If the poller wrote to it, it could overwrite the claim'spendinglock with a terminal state.Gap 3: deployment coverage (
docs/DEPLOYMENT.md, schedule descriptions)Making the poll run on every target means scheduling
finalize_revocationson Fargate, GCP and Container Apps. That is infrastructure work, and #712 already tracks it. Per the issue's acceptance criterion ("or the docs say where it doesn't"), this PR documents the gap instead:docs/DEPLOYMENT.mdsection, "RI Marketplace Listing Poll":finalize_revocations, which is scheduled hourly by default on AWS Lambda, CloudFormation and AKS (AKS only whendeploy_kubernetes_resourcesis true).POST /api/scheduled/finalize_revocationsfrom their own scheduler.enable_finalize_revocations_schedule(Terraformenvironments/aws,modules/compute/aws/lambda,modules/compute/azure/aks) and the CloudFormationEnableFinalizeRevocationsScheduleparameter now say that turning the schedule off also stops the marketplace poll.The issue says the poll does not run on CloudFormation. That is out of date: the
FinalizeRevocationsRuleincloudformation/stacks/CUDly/template.yamlnow schedules it (default on). The coverage test'sknownGaps(terraform/modules/compute/scheduled_task_coverage_test.go) agrees: aws-fargate, gcp-cloud-run, gcp-gke and azure-container-apps are the platforms without it.Concurrency
Each write stays a single guarded
UPDATE. Postgres re-checks theWHEREon the new row version after a lock wait, so a concurrent writer that commits first decides the outcome. Two interleavings matter:ResolveMarketplaceListingAttempt/UpdatePurchaseHistoryListing):cancelled).ResolveMarketplaceListingAttemptalso matches a row that holds the samelisting_id. The cancel is always recorded.pendingagain with the samelisting_id, but it now holds a token, so the token guard drops the poller's write. Without the token guard the poller would turn the claim'spendinglock intoactiveorclosed.A claim can't start on a token-less
pendingrow:ClaimMarketplaceListingSlotrefuses it. So no claim races the pending → active move itself.Verification
All of this is unit, mock and fixture evidence, plus real-Postgres integration tests (testcontainers). No AWS API was called: the poller is tested only with a fake listing finder.
Environment: Go 1.26.9,
AWS_CONFIG_FILE=/dev/null AWS_SHARED_CREDENTIALS_FILE=/dev/null AWS_EC2_METADATA_DISABLED=true.New and changed tests
TestPollMarketplaceListings_PendingRowBecomesActive(unit,internal/api): a pending row that AWS reports active is recorded throughRecordMarketplaceListingActive, with and without aCreateDate.TestPollMarketplaceListings_ActiveRowWithoutCreateDateNeedsNoWriteandTestPollMarketplaceListings_PendingRowSettles(unit).TestMarketplacePendingListingPoll_Postgres(real Postgres, new fileinternal/config/store_postgres_marketplace_pending_poll_integration_test.go):RecordedPendingBecomesActive: covers pending → active with and without aCreateDate, a zero date that must not clearlisted_at, and a foreign listing id that must be ignored.RecordedPendingSettlesMidClaimIsLeftAlone: covers a realClaimMarketplaceListingSlotclaim. Activate and settle must not apply, and the token must be kept.CancelCommittingBeforeActivationWins: a deterministic lock-order race.RelistClaimCommittingBeforePollIsLeftAlone/{activate,settle}: a deterministic lock-order race. The cancel and the claim commit while the poller's write waits on the row.ParallelCancelAndPollKeepCancel: 10 rounds ofResolveMarketplaceListingAttempt, activate and settle started at once. The cancel is always recorded.TestMarketplaceListingSettlement_Postgres/PollSelectionand/SettleIsGuarded:Fail-before / pass-after
origin/main,TestPollMarketplaceListings_PendingRowBecomesActive/create_date_falsefails because no write is made. It passes with the fix.origin/main, these fail:PollSelection,RecordedPendingBecomesActive,RecordedPendingSettles,CancelCommittingBeforeActivationWinsandRelistClaimCommittingBeforePollIsLeftAlone/{activate,settle}. The two race subtests fail because the old guard never matches the pending row, so the write never even waits on its lock. All of them pass with the fix.Mutation probes, each applied alone to the fixed store and then reverted:
listing_client_token IS NULLMidClaimIsLeftAlone,RelistClaimCommittingBeforePollIsLeftAlone/activatelisting_client_token IS NULLMidClaimIsLeftAlone,RelistClaimCommittingBeforePollIsLeftAlone/settle,SettleIsGuardedPollSelectionlistedAtwritten as NULL (noCOALESCE)RecordedPendingBecomesActiveCombined-tree verification at local queue commit
89fe5df6= origin/main3bfc2fe5+fix/issue-1484,fix/issue-1498,fix/issue-1489andfix/issue-1494merged in that order. Passed:go build ./...;go vetwith and without-tags integration;go test -race -short -count=1 ./...; golangci-lint v2.10.1 with and without the integration tag; gocyclo -over 10;go test -race -tags integrationoninternal/api,internal/configandinternal/database/postgres/migrations; the git-secrets scan andscripts/test-git-secrets-allowlist.sh.Out of scope
purchase_historyholds one listing's settlement per RI. A relist (UpdatePurchaseHistoryListing/ResolveMarketplaceListingAttemptwith a new listing id) deliberately clears it, so the new listing can settle.marketplace_listing_settlementstable keyed bylisting_id.listed_at.finalize_revocationson Fargate, GCP and Azure Container Apps. Tracked by fix(iac): several scheduled tasks are not scheduled; delayed purchases never fire on GCP/Azure #712. Only the docs changed here.Frontend follow-up for the UI owner
None needed for this change. Nothing under
frontend/was touched. A listing recorded as pending now becomesactive(orclosed/cancelled) on the next poll, so the history view's existing state display picks it up.Labels
A maintainer please mirror #1494's triage labels:
priority/p2,type/bug. The issue carries noseverity/*,urgency/*,impact/*,effort/*ortriagedlabels.🤖 Generated with Claude Code